Add in-context media asset editing - #2905
Conversation
🦋 Changeset detectedLatest commit: 7fc3022 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Scope checkThis PR changes 2,184 lines across 18 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | f55ad62 | Sep 04 2026, 08:53 PM |
There was a problem hiding this comment.
This PR adds in-context Media Library asset editing to the admin image picker, image fields, and rich-text images/galleries by reusing MediaDetailPanel inside a new useMediaAssetEditor hook, and adds keyboard reordering to gallery blocks. The approach is sound: it stays inside the existing Kumo/dialog and TanStack Query patterns, preserves per-use metadata (alt/caption/layout) when creating cropped copies, and carefully coordinates the picker↔asset-dialog transitions with refs/generations so focus and selection state round-trip correctly.
I read the diff, the full changed source files, the media URL utilities, and the RBAC/role types. I did not run tests/formatting. The main code-level concern is a state-synchronization regression in the rich-text image panel; otherwise the implementation is careful and well-tested. There is also one small a11y/RTL polish item in the dark-variant slot.
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
e3bf84f to
c514a01
Compare
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
bec58f8 to
2e01061
Compare
0d09146 to
7f3be67
Compare
716c425 to
647dd3a
Compare
647dd3a to
d47162e
Compare
d47162e to
f55ad62
Compare
There was a problem hiding this comment.
This re-review of the in-context media asset editing PR finds the approach still sound: the picker↔detail workspace handoff, per-use metadata preservation, and keyboard reordering all fit the existing Kumo/TanStack Query patterns. The prior state-synchronization regression in the rich-text image panel is addressed by keying the ImageDetailPanel reset effect on a stable nodeKey rather than on the attributes object identity, and the added ImageDetailPanel / GalleryDetailPanel tests cover node-switching and cropped-copy preservation.
I read the full diff, the changed source files, the media API/types, and the RBAC role constants. The code is careful and well-tested. The remaining issues are minor a11y/RTL polish items: a couple of decorative icons in newly-added or changed buttons are still missing aria-hidden="true", which leaves them exposed to screen readers alongside the buttons' text/aria-labels. The changeset is useful and the docs update matches the implemented behavior.
No blocking logic, security, or convention regressions were found.
Findings
-
[suggestion]
packages/admin/src/components/ImageFieldRenderer.tsx:376The dark-variant “Add dark mode variant” button renders the decorative
Moonicon without hiding it from assistive technologies. Sibling icons in this file already usearia-hidden="true"; this one should too so screen readers don’t announce both the icon and the button’s visible label.icon={<Moon aria-hidden="true" />} -
[suggestion]
packages/admin/src/components/editor/GalleryDetailPanel.tsx:443The per-image
Replacebutton in the gallery settings card renders theImageSquareicon as visible content. Because the button already has a visible label, the icon is decorative and should be hidden from accessibility APIs to match the other icons in this panel.icon={<ImageSquare aria-hidden="true" />} -
[suggestion]
packages/admin/src/components/editor/GalleryDetailPanel.tsx:392The thumbnail’s remove button has an
aria-label, but the decorativeTrashicon inside it is still exposed to assistive technologies. Addaria-hidden="true"so only the button label is announced.<Trash className="h-3 w-3" aria-hidden="true" />
There was a problem hiding this comment.
I reviewed the full diff for this feature PR, which adds in-context media asset editing to admin image pickers, image fields, and rich-text image/gallery nodes. It also updates MediaPickerModal, MediaDetailPanel, useMediaAssetEditor, plus tests, e2e coverage, docs, and a changeset.
The prior review asked for three decorative icons to be marked aria-hidden, and those are fixed in ImageFieldRenderer. I did not find any logic bugs, regressions, or broken contracts; RBAC checks, state reset keyed on nodeKey, and the intentional cache-only useQuery in GalleryPreviewImage all look reasonable.
I have two minor non-blocking suggestions: a few new decorative icons inside labelled buttons (and the empty-state icons) are still exposed to assistive tech because they lack aria-hidden="true", and the changeset should be tightened to match the repo's style guidance (past tense, one or two sentences).
Findings
-
[suggestion]
packages/admin/src/components/editor/GalleryDetailPanel.tsx:261The
Plusicon in the Add Images button is decorative because the button already has visible text. Decorative icons should be hidden from assistive technology to avoid redundant announcements.icon={<Plus aria-hidden="true" />} -
[suggestion]
packages/admin/src/components/MediaDetailPanel.tsx:1800The
Trashicon in the destructive Delete button is decorative because the visible labelDelete/Deleting...provides the accessible name. Addaria-hidden="true"so screen readers don't also announce the icon.icon={<Trash aria-hidden="true" />} -
[suggestion]
packages/admin/src/components/MediaDetailPanel.tsx:1811The
ArrowsClockwiseicon in the Replace image button is decorative because the button has the visible textReplace image. Hide it from assistive technology witharia-hidden="true".icon={<ArrowsClockwise aria-hidden="true" />} -
[suggestion]
packages/admin/src/components/ImageFieldRenderer.tsx:514The
ImageBrokenicon next to theImage not foundmessage is decorative. It should be hidden from assistive technology to avoid announcing a generic icon name alongside the already-clear text.<ImageBroken className="h-5 w-5" aria-hidden="true" /> -
[suggestion]
packages/admin/src/components/ImageFieldRenderer.tsx:539The
ImageIconicon inside the empty-state Select image button is decorative because the button already has the visible textSelect image. Addaria-hidden="true"to keep the accessible name clean.<ImageIcon className="h-8 w-8" aria-hidden="true" /> -
[suggestion]
.changeset/bright-otters-edit.md:5AGENTS.md asks for changeset entries to be concise (one or two sentences) and in past tense. This entry uses present tense ("Adds") and is three sentences. Consider condensing and converting to past tense, e.g.:
Added in-context Media Library asset editing to admin image pickers, image fields, and rich-text images and galleries, letting editors update metadata and focal points, create cropped copies, replace original image data, and reorder gallery images by keyboard without leaving the content editor.
Thanks!! |
Three conflicting changes landed on main: emdash-cms#2902 (save-conflict recovery), emdash-cms#2891 (publishing and scheduling controls) and emdash-cms#2905 (in-context media editing). Resolutions: - ContentEditor: kept the read-only fieldsets and the `notice` slot, and took every new prop from main -- `publishingState`, `canSchedule`, `isScheduling`, `isUnscheduling`, `onOpenSchedule`, `onUnschedule` and `onMenuOpenChange` on `PublishActions` and `SettingsActionBar`, the schedule props folded into `publishingState` on `ContentSettingsPanel`, and the image-panel guard on the mobile close button. - The save-conflict banner sits inside the disabled fieldset, so an entry held by someone else offers no "Save anyway" the server would refuse. - router: the update mutation's `onError` recovers from a save conflict first, then reports the write error. `isSaveConflict` matches `CONFLICT` and the lock refusal is `ENTRY_LOCKED`, so the two do not overlap. Same order as the autosave mutation, which merged cleanly. - publish-autosave-race: two assertions counted every POST or DELETE, which now includes the lock lease. Narrowed to the schedule endpoint, matching the filters this file already uses elsewhere. Verified: typecheck 0, lint 0, oxfmt clean, core 6393 passed, admin 2025 passed. `editor-save-conflict` (5) and `editor/image-alignment` (2) fail here, but they fail the same way with main's own ContentEditor.tsx and router.tsx checked out in place of ours, so they are not from this merge.
What does this PR do?
Adds in-context Media Library asset editing to admin image workflows:
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Verified on rebased commit
f55ad62f7:pnpm lint:json— 0 diagnosticspnpm lint:quickpnpm typecheckpnpm format